Fix call_workflow samples replay dropping workflow_name during ingestion - #58113
Conversation
…ingestion Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #58113 does not have the implementation label and has only 49 added lines in default business logic directories (<=100 threshold).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — this is a well-diagnosed, minimal fix.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly identified:
call_workflowhad noValidationConfigentry, so ingestion fell back tovalidateItemWithSafeJobConfig, silently droppingworkflow_name/inputs. - ✅ Fix mirrors the existing
dispatch_workflowconfig precisely (same field validation rules), keeping the two safe-output types consistent. - ✅ Includes both a Go unit test (
TestCallWorkflowValidationConfigPreservesWorkflowName) and a JS regression test incollect_ndjson_output.test.cjsthat reproduces the exact reported failure mode (#55176). - ✅ Lock file recompiled to reflect the new validation config.
No actionable issues found in the diff — scope is tight and tests directly cover the regression.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 16.1 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Request changes
This fix plugs the immediate hole, but it still needs one more guardrail in the regression coverage before it is safe to merge.
Blocking theme
The new ingestion test only proves the happy path once call_workflow is already present in the validation config. The original bug happened specifically when that config entry was missing and ingestion fell back to the safe-job path that silently stripped every field except type. Without a test that fails when call_workflow drops out of the generated validation config, this can regress again through another compiler/config wiring path and the suite will still stay green.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
github.comregistry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 33.8 AIC · ⌖ 7.39 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
Strict plain-object validation and invalid-input tests are needed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes samples-mode replay so call_workflow retains its workflow name and inputs during ingestion.
Changes:
- Adds
call_workflowvalidation configuration. - Adds Go and JavaScript regression tests.
- Regenerates the smoke workflow lock file.
File summaries
| File | Review |
|---|---|
pkg/workflow/safe_outputs_validation_config.go |
Adds the validation contract. Moderate: object validation is not enforced; reject arrays and other non-object inputs values and add coverage. |
pkg/workflow/safe_output_validation_config_test.go |
Tests generated validation configuration. |
actions/setup/js/collect_ndjson_output.test.cjs |
Tests preservation of workflow_name and inputs. |
.github/workflows/smoke-call-workflow.lock.yml |
Includes the regenerated validation contract. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| DefaultMax: 1, | ||
| Fields: map[string]FieldValidation{ | ||
| "workflow_name": {Required: true, Type: "string", Sanitize: true, MinLength: 1, MaxLength: 256, Pattern: ".*\\S.*", PatternError: "must not be empty"}, | ||
| "inputs": {Type: "object"}, |
|
🎉 This pull request is included in a new release. Release: |
In samples mode, the dynamically generated
call_workflowMCP tool registers and replays correctly, but the ingestedcall_workflowmessage loses itsworkflow_name/inputs, causing the apply job to fail with "Workflow name is empty".Root cause
pkg/workflow/safe_outputs_validation_config.go'sValidationConfigmap had an entry fordispatch_workflow(declaringworkflow_name/inputsfields) but no equivalent entry forcall_workflow. During ingestion,collect_ndjson_output.cjscheckshasValidationConfig(itemType); when false it falls back tovalidateItemWithSafeJobConfig, which returnsnormalizedItem = { type: item.type }and nothing else whenever the safe-outputs config lacks aninputskey — true forcall_workflow's config (onlymax/workflows/workflow_files). This silently strippedworkflow_nameandinputsbefore the apply handler (call_workflow.cjs) ever saw them.Changes
pkg/workflow/safe_outputs_validation_config.go: added acall_workflowentry toValidationConfigmirroringdispatch_workflow— requiredworkflow_namestring, optionalinputsobject.TestCallWorkflowValidationConfigPreservesWorkflowName(Go) and a vitest case incollect_ndjson_output.test.cjsassertingworkflow_name/inputssurvive ingestion forcall_workflow.smoke-call-workflow.lock.ymlto pick up the new validation config.